Can one of the admins verify this patch?

/assign @balopat

Just for the record I had a similar problem of needing to pass a specific parameter to kubeadm and I implemented a generic approach to solve it: #3879
It has not been merged yet. I am not sure if my approach is better or worse, but I thought it would be relevant to have it mentioned here.

I'd be happy to move it under the --extra-config flag if @marcosdiez 's PR gets approved and other reviewers agree with the approach (I personally think it makes sense to move it there).

#3879 was merged. Should this PR be closed or altered?

@tstromberg I'll be happy to alter it. Do you want to just resolve the conflicts and keep --pod-network-cidr as a top-level minikube param or do you want to move it under --extra-config?

@tstromberg
the existing generic solution (using --extra-conf) done within #3879 does not work for --pod-network-cidr as kubeadm does not allow this parameter to be combined with the --config param. kubeadm allows only following parameters to be used when --config is specified (see kubernetes/cmd/kubeadm/app/apis/kubeadm/validation/validation.go:ValidateMixedArguments):

kubeadmcmdoptions.CfgPath,
kubeadmcmdoptions.IgnorePreflightErrors,
kubeadmcmdoptions.DryRun,
kubeadmcmdoptions.KubeconfigPath,
kubeadmcmdoptions.NodeName,
kubeadmcmdoptions.NodeCRISocket,
kubeadmcmdoptions.KubeconfigDir,
kubeadmcmdoptions.UploadCerts,
kubeadmcmdoptions.CertificateKey,
"print-join-command"
"rootfs",
"v"

The rest of the kubeadm parameters are not allowed and thus will fail if specified in the --extra-conf param in minikube.
Here are some ideas how to fix this:

• update kubeadm to allow all parameters with the --config option (too big of a change change so probably not feasible)
• expose parameters which are not allowed to be used in extra-conf as top-level minikube params, i.e. like this PR does. Probably would be better to drop supporting kubeadm params in extra-conf in this case
• add some handling in minikube that would contain a whitelist of supported kubeadm parameters. The ones that are allowed to be combined with --conf would be propagated to kubeadm as parameters, the rest could be manually added to the config file. E.g. the --pod-network-cidr would not be used as a kubeadm param, but manually put into the config file similarly as done in this PR.

If you would like to keep the --extra-conf support for kubeadm, I find the last option the best. It would fail immediately for unsupported params, is extensible in the future to add additional parameters and would not pollute minikube with parameters from kubeadm.

@11janci - Thanks for the update. Let me give it some thought, as maintaining a whitelist sounds like something that could easily go out of date and cause issues as well. Hmm..

@11janci - I thought about it some more, and agree that the 3rd item makes the most sense. I don't think that particular whitelist will be too much of burden to maintain.

That said, I'm flexible and will accept this PR in either direction. Thank you again for contributing it!

@tstromberg cool thx, I will look into it over the weekend and hopefully come up with a PR.

Please check the failing test for TestGenerateConfig/containerd-pod-network-cidr__default - I think it might not have merged properly with the version bump.

did you rebuilt after the merge? also did you do minikube delete before rerunning? that should work and it does on my machine:

(fyi I rebased off latest master)

Hmm, I must have missed a step somewhere.

Good news, this gets a working flannel setup in host-gw mode:

.\minikube-windows-amd64.exe start --vm-driver=hyperv --network-plugin=cni --enable-default-cni --extra-config=kubeadm.pod-network-cidr=10.244.0.0/16 --extra-config=controller-manager.allocate-node-cidrs=true --extra-config=controller-manager.cluster-cidr=10.244.0.0/16
kubectl apply -f https://gist.githubusercontent.com/PatrickLang/02b7c504fecdd08b498d13bdeee88186/raw/b77ab11e02dba40b597ff6614c401cd71827fa81/kube-flannel.yml

@ksubrmnn this could make the kubeadm adjustments for Windows easier since it's easy to bring up a 1-node cluster with minikube. I still haven't tried joining more nodes yet but it seems like it should work.

/lgtm

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 11janci, PatrickLang
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: balopat

If they are not already assigned, you can assign the PR to them by writing /assign @balopat in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

cc @neolit123 ^^

good to hear, thank you sir :)

can not mix '--config' with arguments [pod-network-cidr]

+

update kubeadm to allow all parameters with the --config option (too big of a change change so probably not feasible)

similar flag -> config overrides might work eventually in the long term (requires component config work). for now we encourage using --config. technically the kubeadm config API (e.g. v1beta1) can be used from a go template which is written to a temp directory and passed via --config.

@PatrickLang

@ksubrmnn this could make the kubeadm adjustments for Windows easier since it's easy to bring up a 1-node cluster with minikube. I still haven't tried joining more nodes yet but it seems like it should work.

👍

With my latest founding we could run minikube + flannel without kubeadm --pod-network-cidr, see

• https://github.com/alvistack/ansible-role-minikube/blob/afdf7b937a26e3e52e2cc68beab86cb6909022d4/tasks/main.yml#L90
• https://github.com/alvistack/ansible-role-minikube/blob/afdf7b937a26e3e52e2cc68beab86cb6909022d4/molecule/ubuntu-18.04/playbook.yml#L34

From https://github.com/coreos/flannel/blob/master/Documentation/troubleshooting.md#kubernetes-specific what we really needed are kubelet's --pod-cidr=<cidr> and controller-manager's --allocate-node-cidrs=true --cluster-cidr=<cidr>, so something like this:

minikube start \
    --extra-config=controller-manager.allocate-node-cidrs=true \
    --extra-config=controller-manager.cluster-cidr=10.233.64.0/18 \
    --extra-config=kubeadm.ignore-preflight-errors=FileContent--proc-sys-net-bridge-bridge-nf-call-iptables,SystemVerification \
    --extra-config=kubelet.cgroup-driver=systemd \
    --extra-config=kubelet.network-plugin=cni \
    --extra-config=kubelet.pod-cidr=10.233.64.0/18 \
    --extra-config=kubelet.resolv-conf=/run/systemd/resolve/resolv.conf \
    --kubernetes-version=v1.14.1 \
    --network-plugin=cni \
    --vm-driver=none

curl -skL https://raw.githubusercontent.com/coreos/flannel/master/Documentation/kube-flannel.yml > /tmp/kube-flannel.yml
sed -i 's/10.244.0.0\/16/10.233.64.0\/18/g' /tmp/kube-flannel.yml
kubectl apply -f /tmp/kube-flannel.yml

New changes are detected. LGTM label has been removed.

Hi @tstromberg ,
thx for your review, PR updated, PTAL.
It is working as expected and is ready to ship.

there is a unrelated unit test failing in the build (TestNewSSHClient), it's failing in master as well. Needs to be resolved first
resolved